Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a kola verb #85

Merged
merged 1 commit into from
May 29, 2019
Merged

Add a kola verb #85

merged 1 commit into from
May 29, 2019

Conversation

cgwalters
Copy link
Member

@cgwalters
Copy link
Member Author

Now blocked on coreos/mantle#918

@cgwalters cgwalters closed this Sep 18, 2018
@cgwalters cgwalters reopened this Sep 18, 2018
@dustymabe dustymabe added the WIP PR still being worked on label Sep 19, 2018
@cgwalters cgwalters changed the title WIP: Add a kola verb Add a kola verb Sep 21, 2018
@cgwalters cgwalters removed the WIP PR still being worked on label Sep 21, 2018
@cgwalters
Copy link
Member Author

Lifting WIP on this, but we have a dependency in coreos/mantle#920 at least...there's general nontrivial work to teach kola about the 3 distributions.

@cgwalters
Copy link
Member Author

coreos/mantle#922

@arithx
Copy link
Contributor

arithx commented Oct 10, 2018

coreos/mantle#922 has landed (I had forgotten to actually merge it)

@dustymabe
Copy link
Member

should we wait on the discussion in #163 since i think it has implications?

poor PR :(

@jlebon
Copy link
Member

jlebon commented May 16, 2019

So now with #524, let's (1) switch this to run the unpriv tests, (2) get it in, and (3) update coreos/fedora-coreos-pipeline#31 to leverage it?

@jlebon
Copy link
Member

jlebon commented May 16, 2019

should we wait on the discussion in #163 since i think it has implications?

I'm not sure if that discussion necessarily blocks this patch though, right? We can assume we have kola in the container, and we can discuss in that ticket how it gets in there. :)

@dustymabe
Copy link
Member

should we wait on the discussion in #163 since i think it has implications?

I'm not sure if that discussion necessarily blocks this patch though, right? We can assume we have kola in the container, and we can discuss in that ticket how it gets in there. :)

I think at some point during that discussion it was up in the air whether we wanted it to be a completely separate container or not. I think at this point it is safe to assume regardless of how we build it we will include it in cosa. I agree that we don't need to block.

src/cmd-kola Outdated
fatal "No latest build"
fi

exec sudo kola -b rhcos --tapfile test.tap --qemu-image ${latest_qcow} "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is -b rhcos important here? is there a -b fcos ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Distribution flags are important. That said, we should make it a flag on the command and just have it default to one of fcos/rhcos

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd vote for changing kola to look for rhcos- or fcos- as a prefix in the image name for now; that same thing would also be useful for ignition spec version handling.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefix based detection WFM.

And yes, when --ignition-version v2 will need to be specified when running RHCOS until spec 3.0.0 lands there. For FCOS we don't need to specify that parameter as it defaults to v3 on that distribution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would probably be good to have command-line overrides as well (at least on Ignition version) so that it would be possible to test Ignition spec 3.0.0 in RHCOS without having to modify cmd-run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the auto-detect logic in the command itself for now with a XXX to upstream it into kola.

@jlebon jlebon force-pushed the cmd-test branch 4 times, most recently from 17a0dec to 859be8c Compare May 24, 2019 21:39
@jlebon
Copy link
Member

jlebon commented May 24, 2019

OK, I rebased this now!

$ ca kola run -p qemu-unpriv
...
+ exec sudo kola -b fcos --tapfile test.tap --qemu-image builds/30.8/fedora-coreos-30.8-qemu.qcow2 run -p qemu-unpriv
=== RUN   fcos.basic
=== RUN   rpmostree.upgrade-rollback
=== RUN   coreos.selinux.enforce
=== RUN   coreos.ignition.sethostname
=== RUN   rpmostree.status
=== RUN   coreos.auth.verify
=== RUN   coreos.ignition.groups
=== RUN   coreos.selinux.boolean
=== RUN   rhcos.selinux.boolean.persist
=== RUN   coreos.ignition.once
=== RUN   systemd.sysusers.gshadow
=== RUN   fcos.basic/ReadOnly
=== RUN   fcos.basic/Useradd
=== RUN   fcos.basic/MachineID
=== RUN   fcos.basic/PortSSH
=== RUN   fcos.basic/DbusPerms
=== RUN   fcos.basic/ServicesActive
--- PASS: fcos.basic (29.76s)
    --- PASS: fcos.basic/ReadOnly (0.18s)
    --- PASS: fcos.basic/Useradd (0.69s)
    --- PASS: fcos.basic/MachineID (0.18s)
    --- PASS: fcos.basic/PortSSH (0.21s)
    --- PASS: fcos.basic/DbusPerms (0.43s)
    --- PASS: fcos.basic/ServicesActive (0.24s)
--- PASS: coreos.ignition.groups (26.28s)
--- PASS: rhcos.selinux.boolean.persist (47.87s)
--- PASS: coreos.selinux.boolean (44.80s)
--- PASS: coreos.ignition.sethostname (25.18s)
--- PASS: coreos.auth.verify (28.04s)
--- PASS: rpmostree.status (25.78s)
--- PASS: systemd.sysusers.gshadow (26.54s)
--- PASS: coreos.selinux.enforce (45.81s)
=== RUN   rpmostree.upgrade-rollback/upgrade
=== RUN   rpmostree.upgrade-rollback/rollback
--- PASS: rpmostree.upgrade-rollback (76.25s)
    --- PASS: rpmostree.upgrade-rollback/upgrade (24.51s)
    --- PASS: rpmostree.upgrade-rollback/rollback (26.50s)
--- PASS: coreos.ignition.once (43.54s)
PASS, output in tmp/kola

Let's get this in?

@jlebon
Copy link
Member

jlebon commented May 24, 2019

(Though I actually didn't test it on RHCOS yet.)

@jlebon jlebon force-pushed the cmd-test branch 2 times, most recently from d9a8b27 to 3577116 Compare May 24, 2019 22:29
src/cmdlib.sh Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member Author

This looks sane to me!

@jlebon jlebon removed the hold waiting on something label May 27, 2019
Copy link
Contributor

@arithx arithx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM, couple questions.

src/cmd-kola Outdated
set -x

# shellcheck disable=SC2086
exec sudo kola $distro --output-dir tmp/kola --tapfile test.tap --qemu-image "${latest_qcow}" "$@"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have anything keying on the tapfile in this command, maybe drop it and let things specify it directly if they care about it?

Might be worthwhile to default to some extra parallel threads (via --parallel <num>).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, dropped the --tapfile bit!

Might be worthwhile to default to some extra parallel threads (via --parallel <num>).

I left this off for now. It's tricky to get right to match all possible environments. See e.g. get_libvirt_smp_arg in virt-install. Hmm, though I guess I could split that out into a separate Python script and here shell out to it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably fair to leave out parallel for now, it can be specified on the command-line and has the extra trickery around it as you mentioned.

@miabbott
Copy link
Member

(Though I actually didn't test it on RHCOS yet.)

qemu_unpriv will likely fail with RHCOS due to https://github.com/coreos/mantle/issues/999

@arithx
Copy link
Contributor

arithx commented May 28, 2019

(Though I actually didn't test it on RHCOS yet.)

qemu_unpriv will likely fail with RHCOS due to coreos/mantle#999

This verb doesn't specify the platform (which defaults to qemu not qemu-unpriv).

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to make cmd-kola executable.. otherwise you get: Unknown command: kola

src/cmd-kola Outdated Show resolved Hide resolved
src/cmd-kola Outdated Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented May 29, 2019

need to make cmd-kola executable

Huh, I'm not sure how I didn't hit that... anyway, fixed!

latest=$(get_latest_build)
if [ -n "$latest" ]; then
# shellcheck disable=SC2086
ls builds/${latest}/*-qemu.qcow2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not anything we need to do here but FYI if you have run cosa compress this will fail.

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dustymabe
Copy link
Member

LGTM

if only shellcheck were happy

This is the first place we're wrapping mantle via our
main entrypoint, and hopefully we'll increase that.

Example usage: `coreos-assembler kola run rhcos.basic`.

Requires: coreos/mantle#920
@jlebon
Copy link
Member

jlebon commented May 29, 2019

OK right, this code predates the addition of ShellCheck. Should hopefully be happy now!

@dustymabe
Copy link
Member

haha - i just thought about it cosa kola is very close to coca cola :)

@dustymabe dustymabe merged commit 0c4fb22 into coreos:master May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants